Fail out when intermediate certificate chain fails to load #9230
Conversation
| if (!SSL_CTX_add_extra_chain_cert_bio(ctx, bio.get())) { | ||
| Debug("ssl", "couldn't add chain to %p", ctx); | ||
| SSLError("failed to load intermediate certificate chain from %s", cert_names_list[i].c_str()); | ||
| return false; |
There was a problem hiding this comment.
Clearly this should be an Error. But, if you look at the call stack this function is at the bottom of, it's not clear whether or not it's better to return false here. I assume none of these errors cause Fatal() calls because of the reload case. TS code tends avoid exiting on a reload error. Without necessarily considering it may be worse to keep running in a corrupt state.
Unfortunately, in industrial source bases, even those relied on by established, successful companies, it's risky to assume that something sensible will be done with an error return from a function. https://www.youtube.com/watch?v=lt-udg9zQSE
There was a problem hiding this comment.
This error handling replicates the behavior of all other errors (i.e. secret_data.empty() and when cert fails to load certificate chain). Similar to those, it should exit out of that function immediately
There was a problem hiding this comment.
Have you seen this happen in production?
There was a problem hiding this comment.
No, not on this specific error. This was the only error case that didn't add a SSLError and exit with returning false
There was a problem hiding this comment.
Hopefully someone who knows this code better than I do will take a quick look.
shinrich
left a comment
shinrich
left a comment
There was a problem hiding this comment.
I agree, it should fail in this case.
I looked through the code a bit to see about the returning 0 or not. Someone should probably do some experiments here. By my reading, the main (only) caller of load_certs is init_server_ssl_ctx which returns an array of SSLLoadingContext. If there is a failure in load_certs, the return list will be empty. Since the function doesn't distinguish between a failure and no items, I think we lose the error at that point.
Sound probably do some experiments and refactor that path to propagates the errors too in a separate PR.
|
@shinrich I agree, I think the error handling for all of those errors should be done in a separate effort/PR |
Co-authored-by: Serris Lew <lserris@apple.com> (cherry picked from commit e1f2f5b)
|
Cherry-picked to v9.2.x |
* asf/9.2.x: Updated ChangeLog Remove out of bound SSL log (apache#9236) Fail out when intermediate certificate chain fails to load (apache#9230) Revert "9.2: Fix s3_auth_config test output check (apache#9123)" (apache#9237)
4 participants
ATS should immediately fail out when intermediate certificate chains fail to load. Currently only a debug log is included when this fails. It originally failed out (by calling
goto fail) but was missed during refactoring in #853SSL_CTX_add_extra_chain_cert_fileis called, which fails out when returning an error